refactor(nix): use native Bun APIs and propagate errors #12694
Conversation
Use safeReadDir and isDirectory helpers (consistent with normalize-bun-binaries.ts) and replace dynamic semver import with Bun.semver.order. Fixes anomalyco#12632, anomalyco#12603, anomalyco#12602
|
The following comment was made by an LLM, it may be inaccurate: No duplicate PRs found (PR #12636 appears in the results, but that's the related "gigamonster's build fix" that this PR is explicitly a follow-up to, not a duplicate.) |
There was a problem hiding this comment.
Pull request overview
Refactors the Nix Bun node_modules post-processing scripts to rely more on Bun-native APIs and (intended) stronger error propagation during canonicalization/normalization of .bun-managed modules.
Changes:
- Replaced
safeReadDir()usage with directreaddir()calls innormalize-bun-binaries.ts. - Switched canonicalization version ordering to
Bun.semver.orderand introduced anisDirectory()helper for directory checks. - Updated normalize script logging wording (“rewrote” → “rebuilt”).
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| nix/scripts/normalize-bun-binaries.ts | Removes safeReadDir() and uses direct readdir() calls when scanning .bun and package directories; adjusts log message. |
| nix/scripts/canonicalize-node-modules.ts | Removes dynamic semver import, uses Bun.semver.order for selection sorting, and refactors directory checks via isDirectory(). |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| const semverModule = (await import(join(bunRoot, "node_modules/semver"))) as | ||
| | SemverLike | ||
| | { | ||
| default: SemverLike | ||
| } | ||
| const semver = "default" in semverModule ? semverModule.default : semverModule | ||
| const selections = new Map<string, Entry>() | ||
|
|
||
| for (const [slug, list] of versions) { | ||
| list.sort((a, b) => { | ||
| const left = semver.valid(a.version) | ||
| const right = semver.valid(b.version) | ||
| if (left && right) { | ||
| const delta = semver.rcompare(left, right) | ||
| if (delta !== 0) { | ||
| return delta | ||
| } | ||
| } | ||
| if (left && !right) { | ||
| return -1 | ||
| } | ||
| if (!left && right) { | ||
| return 1 | ||
| } | ||
| return b.version.localeCompare(a.version) | ||
| }) |
There was a problem hiding this comment.
sure, using the bun semver module is nice, but why remove the checks for non semver package versions? afaict, the behavior of semver.order is not specified for invalid semver strings and could lead to nondeterministic sorting: https://bun.com/docs/runtime/semver
There was a problem hiding this comment.
Fixed in fec6dd2 - added isValidSemver() guard to handle invalid versions (latest, canary, git URLs) before calling Bun.semver.order.
There was a problem hiding this comment.
are you sure Bun.semver.order throws when provided invalid semver string? why not const isSemver = (s) => Bun.semver.satisfies(s, "x.x.x") which is documented to return false when s is not a semver string
There was a problem hiding this comment.
Bun.semver.order throws on invalid versions (e.g. Invalid SemVer: latest), so the try/catch worked. Switched to Bun.semver.satisfies(v, "x.x.x") which is cleaner—returns false without exceptions. Fixed in bfb3a4a.
jerome-benoit
force-pushed
the
fix/nix-missing-bun-dir-v2
branch
from
3763b4f to
594fdb0
Compare
jerome-benoit
force-pushed
the
fix/nix-missing-bun-dir-v2
branch
from
205fafb to
538560c
Compare
3 participants
Follow-up to #12636.
safeReadDir()withreaddir()to propagate errors instead of returning empty arrays silentlyBun.semver.orderinstead of external semver importisDirectory()helper for cleaner checksRelated: #12632 #12603 #12602